Discard counters for out-of-bounds levels #975
Conversation
|
|
Previously this just crashed.
|
Please take a look - is that what you had in mind?
…On Wed, Jun 30, 2021 at 2:50 PM Prashant Varanasi ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In zapcore/sampler.go
<#975 (comment)>:
> func (cs *counters) get(lvl Level, key string) *counter {
i := lvl - _minLevel
+ if i < 0 {
+ return &nullCounter
My alternative proposal was mostly to localize this change to where we use
the level to convert to an index, but I agree that affecting other levels
may be surprising.
I think the preferred behaviour is for the sampler to ignore unknown
levels. Instead of making any change here, we can instead do the level
check at the caller,
https://github.com/uber-go/zap/blob/v1.18.1/zapcore/sampler.go#L200
If the level is unsupported, then we "fail open" by calling the underlying
core's Check.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#975 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVFUG2T4N55BWENNKSTTVOGQ5ANCNFSM47PEQ2YQ>
.
|
Codecov Report
@@ Coverage Diff @@
## master #975 +/- ##
=======================================
Coverage 98.10% 98.10%
=======================================
Files 46 46
Lines 2056 2057 +1
=======================================
+ Hits 2017 2018 +1
Misses 30 30
Partials 9 9
Continue to review full report at Codecov.
|
|
Hello. Any chance we can get a new tag, so I can pin to this version? |
| if n > s.first && (n-s.first)%s.thereafter != 0 { | ||
| s.hook(ent, LogDropped) | ||
| return ce | ||
| if ent.Level >= _minLevel && ent.Level <= _maxLevel { |
There was a problem hiding this comment.
Maybe I'm dropping here like a martian, but wouldn't it make more sense to clip the ent.Level to [_minLevel, _maxLevel] range?
Will this behavior cause log entries outside valid range to bypass sampling, thus always be emitted?
There was a problem hiding this comment.
That was one of the options considered, see #975 (comment) but there was concern that it could confuse users.
Yes, the current approach causes log entries with unknown levels to bypass sampling.
There was a problem hiding this comment.
In typical usecase for this feature would be integration with klog/logr. Those out-of-bounds entries are going to be the most common ones actually, and ones which should be sampled...probably....maybe when user actually uses those levels they are interested in all logs and will bypass sampling anyway?
|
My feeling is that super-verbose levels are special and not "normal" and,
as such, sampling is probably not appropriate.
That's how I justify it to myself anyway :)
BTW: How about a new tag ? :)
|
|
That was my feeling. logr V(1) == zap.Debug (so sampled). logr V(2) ==
zap.Level(-2). Now I will admit that some k8s components run at V=2 by
default, but that itself seems like a problem :)
…On Thu, Aug 5, 2021 at 11:36 PM Neven Miculinic ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In zapcore/sampler.go
<#975 (comment)>:
> @@ -197,12 +197,14 @@ func (s *sampler) Check(ent Entry, ce *CheckedEntry) *CheckedEntry {
return ce
}
- counter := s.counts.get(ent.Level, ent.Message)
- n := counter.IncCheckReset(ent.Time, s.tick)
- if n > s.first && (n-s.first)%s.thereafter != 0 {
- s.hook(ent, LogDropped)
- return ce
+ if ent.Level >= _minLevel && ent.Level <= _maxLevel {
In typical usecase for this feature would be integration with klog/logr.
Those out-of-bounds entries are going to be the most common ones actually,
and ones which should be sampled...probably....maybe when user actually
uses those levels they are interested in all logs and will bypass sampling
anyway?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#975 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVH2IAIBPZ7HPQNSFPLT3N7FBANCNFSM47PEQ2YQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
Oops, missed this. We'll tag a release next week. (We prefer not to tag new releases of core components on Fridays.) Thanks, @thockin! |
|
Thanks!
…On Fri, Aug 6, 2021 at 11:38 AM Abhinav Gupta ***@***.***> wrote:
BTW: How about a new tag ? :)
Oops, missed this. We'll tag a release next week. (We prefer not to tag
new releases of core components on Fridays.) Thanks, @thockin
<https://github.com/thockin>!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#975 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVFVI5O746LPZEPK5QTT3QT35ANCNFSM47PEQ2YQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
Previously this just crashed.
Fixes #972